SpeedyWeather v0.19 compat#133
Conversation
|
Ah apparently we were restricting the version of SpeedyWeather, that is why the tests were still passing |
|
I think right at the moment that's actually also still a fair assumption to restrict versions. One of the next releases of SpeedyWeather will also def break this again, as we are rewriting the variable system. So restricting the version ensures it actually still works. |
|
Ok, so shall we keep this PR open such that we can update SpeedyWeather in one go when you guys finished with the variables refactor and in the meantime just add a warning when we load the extension if v > 0.17? |
|
I don't know. I mean this here is just a quick fix, and makes it run with v0.18. Isn't that good in itself? We'll likely merge the new variable system next week or in two weeks and then wait a bit more for a proper release, and then we could just another small PR. It's just a few lines of code that change here. |
|
I think the compat entry for v0.18.1 that this PR adds is appropriate. |
|
@maximilian-gelbrecht, after the SpeedyWeather v0.18.1+ that this PR introduces, does it make sense to run the atmosphere in the global_cloupled_simulation.jl example on the GPU? |
|
As of now, the GPU version is running and also part of the released version, but we don't really consider it fully finished. We invested a good amount of time into the parametrizations, but the dynamical core still needs some optimisations to run more efficiently on GPU. |
|
Cool; let's leave transitioning the atmosphere part of the example to GPU for later. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I realised this actually needs a few more minor changes due to other changes to the variables system we did since 0.17.4, not only the signature of the Nothing really major on the NumericalEarth side here, as far as I can see it. Will address those later. |
|
Let me know when it’s ready to have a look |
|
|
||
| Base.summary(::SpeedySimulation) = "SpeedyWeather.Simulation" | ||
|
|
||
| # TODO: This is a temporary solution to tell SpeedyWeather to allocate the ocean variables, without having an ocean. In SpeedyWeather PR#969 it will be addressed slightly more properly. |
There was a problem hiding this comment.
I guess in the future we want to have ocean=nothing in speedyweather and then we just compute everything here?
There was a problem hiding this comment.
This is just there because we introduced the first of part of new variable system in 0.18, and it was an oversight of us to not always allocate the sea_surface_temperature somewhere, even with ocean=nothing. SpeedyWeather/SpeedyWeather.jl#969 is the second part of that. We just have to decide where to allocate the sea_surface_temperature (and similar variables for land and sea ice). Once we do that, you can probably do ocean=nothing again.
So yeah, this is just about memory allocation, and not about computations. Those are all here / in Oceananigans anyway.
There was a problem hiding this comment.
We went with PrescribedOcean() for now. I think this also makes sense in this case. The ocean isn't non-existent / nothing it is prescribed by NumericalEarth. PrescribedOcean just makes sure that we have the proper variables allocated to be written into by NumericalEarth.
…ns.jl Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
|
I just updated this to version 0.19 that we just released. I still have to check if it actually works. Locally, I have some trouble running the tests because of a version conflict between Oceananigans and NumericalEarth in the test env. I am a bit confused by that. |
|
My update is just adjusting for syntax changes. We could probably also think about actually using ConservativeRegridding in a follow up, or is it still not ready? Edit: Ah, I see that's already part of #160 |
Co-authored-by: Copilot <copilot@github.com>
|
Sorry, this took me a bit, but now the Speedy coupling test passes locally. |
|
I think this is ready to merge |
|
Tests are not passing because this is a fork that does not have access to the credentials which are repository secrets. I will merge anyways because the speedy weather coupling tests pass |
Fixes #132